Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Try retry the download when CURLOpen fail on stream download #594

Merged
merged 1 commit into from
May 15, 2021
Merged

Try retry the download when CURLOpen fail on stream download #594

merged 1 commit into from
May 15, 2021

Conversation

CastagnaIT
Copy link
Collaborator

I have noticed that on days with heavy server traffic (on december/jennuary for christmas days/new year's days) the stream download sometime failed causing unexpected behaviour to Kodi like my issue #583

the problem is that when CURLOpen fails the download will be interrupted
so let's make at least 2 attempts before breaking the playback

MPParsley
MPParsley reviewed Jan 31, 2021
View reviewed changes
src/main.cpp Outdated Show resolved Hide resolved
MPParsley
MPParsley approved these changes Feb 2, 2021
View reviewed changes
Copy link

@MPParsley MPParsley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be the cause for:

2021-02-02 20:56:20.607 T:1762808032 WARNING: AddOnLog: InputStream Adaptive: Initializing stream with unknown KID!
2021-02-02 20:56:21.843 T:1762808032   ERROR: CCurlFile::FillBuffer - Failed: HTTP returned error 403
2021-02-02 20:56:21.843 T:1762808032   ERROR: CCurlFile::Open failed with code 403 for https://wv-keyos.licensekeyserver.com/:

@CastagnaIT
Copy link
Collaborator Author

@MPParsley for error 403 sometimes means problem with DNS used or you are under VPN and the server refuse your connection, perhaps you can also try check possible http headers to add

@MPParsley
Copy link

@MPParsley for error 403 sometimes means problem with DNS used or you are under VPN and the server refuse your connection, perhaps you can also try check possible http headers to add

Thanks for the tip @CastagnaIT!

In my case it's probably a real 403 since there's no KID, I opened another issue for it on #599

@glennguy glennguy mentioned this pull request Mar 4, 2021
Closed
@phunkyfish phunkyfish added the Issue Type: New feature issue has requested a new feature label Mar 31, 2021
@glennguy
Copy link
Contributor

glennguy commented May 7, 2021

@CastagnaIT I don't think this is the right solution to the problem. AdaptiveStream already has a segment retrying capability, just that it isn't enabled for VOD streams:

inputstream.adaptive/src/common/AdaptiveStream.cpp

Lines 92 to 97 in 99d72cd

while (!ret && !stopped_ && retryCount-- && tree_.has_timeshift_buffer_)
{
std::this_thread::sleep_for(std::chrono::seconds(1));
Log(LOGLEVEL_DEBUG, "AdaptiveStream: trying to reload segment ...");
ret = download_segment();
}

My guess would be that 404s are quite common on the very edge of a live stream but it's assumed (obviously not the case though) that VOD won't ever 404 or fail otherwise.
If you remove the live stream requirement then all should be good :)

@CastagnaIT
Copy link
Collaborator Author

oh thanks i will do

@arnova
Copy link
Member

arnova commented May 7, 2021

This looks like a ugly hack. This implicates that if we need to retry because a server is already overloaded, we hammer it even more. And still: If we want something like this, I think it should go into CCurlfile where one has greater control over when this should kick in.

@CastagnaIT
Copy link
Collaborator Author

CastagnaIT commented May 14, 2021 •
edited
Loading

@glennguy done, but i not understand why one build test is failed...
@arnova this PR is only to improve existing behaviour
if you wish develop/review all the code that manage the requests "retry" in a better way is better do it into another PR

@arnova
Copy link
Member

arnova commented May 14, 2021

@glennguy done, but i not understand why one build test is failed...
@arnova this PR is only to improve existing behaviour
if you wish develop/review all the code that manage the requests "retry" in a better way is better do it into another PR

Agreed.

glennguy
glennguy approved these changes May 14, 2021
View reviewed changes
Copy link
Contributor

@glennguy glennguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

matthuisman
matthuisman reviewed May 14, 2021
View reviewed changes
Copy link
Contributor

@matthuisman matthuisman left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsigned int retryCount(10);

Is 10 retries too many?
Do we need delay between?
Could we do 10 for live, 3 for vod for example?

@glennguy
Copy link
Contributor

I think the 1 second delay is appropriate, no delay would be just hammering the server.

Remember the issues with Kayo - 1 second delay and retry was usually enough to get it resolved. The only time I see multiple retries in a log is when there's another issue causing wrong segment number or filename calculation, then we get the full 10 retries due to 404 and stream eventually stops.

10 was probably a number plucked out of the air and could be reduced.

Is this outside the scope of the PR though (let VOD streams have the functionality to retry)?

@matthuisman
Copy link
Contributor

matthuisman commented May 15, 2021 •
edited
Loading

Is there already a delay here that missed?

Or maybe its better to retry based on total time taken. So a slow server may only retry 3 times. But fast server could retry 10

Anyway. Something to keep in mind.
Looks good to me

@glennguy glennguy merged commit fbd7310 into xbmc:Matrix May 15, 2021
@glennguy glennguy mentioned this pull request May 15, 2021
Closed
@CastagnaIT CastagnaIT deleted the retry_download branch June 19, 2021 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glennguy glennguy glennguy approved these changes

@matthuisman matthuisman matthuisman approved these changes

@MPParsley MPParsley MPParsley approved these changes

Assignees
No one assigned
Labels
Issue Type: New feature issue has requested a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@CastagnaIT @MPParsley @glennguy @arnova @matthuisman @phunkyfish